-
Notifications
You must be signed in to change notification settings - Fork 2
chore(NET-112): Add http api, streams and query related metrics to HotblockDB #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| pub fn spawn_metrics_reporter(&self, interval: Duration) { | ||
| let active_count = self.in_flight.clone(); | ||
|
|
||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just increase/decrease this one every time when the active_count gets updated?
Also what if there are multiple query executors in the process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just increase/decrease this one every time when the active_count gets updated?
I agree that’s possible. The reasoning here: we already have one shared atomic (active_count) that’s updated in a few places and has guardrails/clamping to keep it within bounds. If we also manually increment/decrement a separate metric counter in the same places, that’s extra duplication and increases the chance the metric drifts from the actual value over time (especially if another update site gets added later).
With the current approach, the metric just reflects the authoritative value after it changes. Can you clarify your concern?
Also what if there are multiple query executors in the process?
From what I can tell the code isn’t currently structured/documented to support multiple QueryExecutors. And even if we did have multiple instances, sqd_polars::POOL is shared across them anyway, so it’s not clear that multiple executors would give isolation or change the semantics of the metric much.
Are you expecting a multi-executor setup where each instance should report separate metrics (per executor), or is the intent a single process-wide metric?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive metrics instrumentation to HotblocksDB to track HTTP API performance, stream statistics, and query execution metrics. The implementation introduces Prometheus-compatible metrics for monitoring query throughput, data transfer rates, and API endpoint usage.
Changes:
- Added HTTP middleware to track request metrics including status codes, time-to-first-byte, and endpoint-specific labels
- Implemented stream and query statistics tracking with histograms for bytes, blocks, chunks, and throughput rates
- Integrated metrics reporter that periodically updates active query gauges
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/hotblocks/src/metrics.rs | Added new metric definitions for HTTP, stream, and query tracking; reorganized registry with lazy_static |
| crates/hotblocks/src/query/executor.rs | Added metrics reporter for active queries and completed query counter |
| crates/hotblocks/src/query/running.rs | Implemented RunningQueryStats to track chunks, blocks, and bytes processed per query |
| crates/hotblocks/src/query/response.rs | Added QueryStreamStats to track stream metrics with time limit support and metrics reporting on drop |
| crates/hotblocks/src/query/service.rs | Exposed spawn_metrics_reporter method for periodic metrics updates |
| crates/hotblocks/src/api.rs | Added middleware for HTTP request tracking, ResponseWithMetadata pattern for endpoint labeling, and 404 handler |
| crates/hotblocks/src/cli.rs | Initialize metrics reporter with 5-second interval |
| crates/hotblocks/src/dataset_config.rs | Added clarifying comments to RetentionConfig variants |
| crates/hotblocks/Cargo.toml | Added lazy_static and tower-http dependencies |
| Cargo.lock | Updated lock file with new dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What is this PR about?
Closes: https://linear.app/sqd-ai/issue/NET-112/add-metrics-to-hotblocksdb
PR adds metrics for main http endpoints, as well as stream/query related information for HotblocksDB.
Metrics example from the local run
Some of the style changes may appear excessive due to rustfmt not having been run previously.